Implement namespacing for doc comments IDs #92043
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GuillaumeGomez
added
the
T-rustdoc
rust-highfive
added
the
S-waiting-on-review
GuillaumeGomez
commented
Dec 17, 2021
src/librustdoc/html/markdown.rs
Outdated
| @@ -1,29 +1,6 @@ | |||
| //! Markdown formatting for rustdoc. | |||
| //! | |||
| //! This module implements markdown formatting through the pulldown-cmark library. | |||
| //! | |||
There was a problem hiding this comment.
I can't keep this doc comment after my update because IdWrapping has clean::Item which cannot be reexported. Also, I couldn't find out why it was there in the first place...
There was a problem hiding this comment.
rustdoc::html::markdown is used by the error-index-generator. Perhaps that's why these docs were here?
There was a problem hiding this comment.
You are right, this is very surprising that it's done this way... (It's in src/tools/error_index_generator.) I wonder why it's not using the rustdoc flag directly instead.
There was a problem hiding this comment.
That's a good point. I wonder if it needs to control some rustdoc internals?
There was a problem hiding this comment.
I'll try to change it to see what the result looks like.
There was a problem hiding this comment.
They provide their own style files. So better keep it as is for now.
jsha
reviewed
Dec 17, 2021
There was a problem hiding this comment.
Generally looks good! Note that HeadingOffset and IdWrapping are conceptually related - they both give markdown information about the immediate parent heading. So maybe combine them into one ParentInfo that contains both?
Instead of calling it IdWrapping, how about IdPrefix?
And instead of containing Some(&' clean::Item), what about containing an &'a str, which is the prefix itself? That narrows down the information available through this and makes its purpose clearer.
That would force us to make this transformation everywhere, which doesn't seem like a good idea. I prefer when a same operation from multiple places is also done in one place (so the
Sounds good to me.
I'll take a look into that, maybe it could be shortened indeed! |
What about this: |
|
Hmm, I just realized that this will of course break all links that users have to their Markdown headings. Is that acceptable breakage? I feel like we should probably have more team discussion and an FCP. |
|
One possibility - we might be able to write JS that detects a fragment that doesn't exist in the doc, and searches the actually-existing IDs for what it would have been. So for instance if we get We probably wouldn't want to keep this forever, but we could keep it around for a couple of years so there's time to transition. Also note that the existing links are not very stable. For instance adding a new method with its own Also since old versions on docs.rs don't usually get regenerated, the amount of breakage in links to old versions will be minimal. |
Hmm, but then no one would transition 🙃 I think it's okay to break the links to headings that are within methods since they're basically broken already. But I wonder if there's a way to mitigate the breakage for top-level (e.g., struct-level) headings since they're not already broken? I'm going to tag this with relnotes since it should probably be in the compatibility notes, and I think we should do some more discussion on the issue and then have an FCP. |
camelid
added
relnotes
|
@shepmaster you were saying this wasn't the solution you expected, right? Would it be a big deal to add a prefix for your markdown anchors / do you have a better solution in mind? |
|
@GuillaumeGomez could you post a demo of the stdlib docs with this change so we can check various edge cases? |
|
Sure, here it is: https://rustdoc.crud.net/imperio/doc-comment-id-namespace/std/index.html |
I'm not sure that I fully knew what to expect 😉 That said, I am surprised that it namespaces the user-supplied data. I guess I expected it to namespace the rustdoc-supplied data. That is, I expected this link:
to be I also expected this link:
to be
That rustdoc adds or that I add? |
|
GuillaumeGomez
force-pushed
the
doc-comment-id-namespace
branch
from
eb29a90
to
c2aabd6
Compare
|
Updated to keep the current error index generator working. |
This comment has been minimized.
This comment has been minimized.
|
I also updated the GUI tests. However, considering how big the change is, I put it into its own commit. |
This comment has been minimized.
This comment has been minimized.
|
Needs #91834 to be merged first... |
camelid
reviewed
Dec 18, 2021
src/librustdoc/html/markdown.rs
Outdated
| pub struct IdPrefix<'a>(IdPrefixInner<'a>); | ||
|
|
||
| impl<'a> IdPrefix<'a> { | ||
| pub fn without_parent() -> Self { |
|
☔ The latest upstream changes (presumably #92090) made this pull request unmergeable. Please resolve the merge conflicts. |
GuillaumeGomez
force-pushed
the
doc-comment-id-namespace
branch
from
368b4c2
to
232dee6
Compare
|
Updated! |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
So before I update all the broken anchors in the docs, I'll wait for confirmation the current code is good. ^^' |
camelid
added
the
needs-fcp
|
cc @jsha |
|
☔ The latest upstream changes (presumably #91948) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this. @rustbot label: +S-inactive |
rustbot
added
the
S-inactive
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #91759.
I think it also completely removes the need for #86178 (yeay!).
Thanks to the namespacing, we can stop worrying about declaring the IDs used by rustdoc for its layout.
You can check it online here.
cc @jsha (since the idea comes from you :) )
r? @camelid